Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Concat document definitions #105

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

evanrs
Copy link

@evanrs evanrs commented Jun 21, 2017

Document fragments parsed by graphql-tag/loader lose their definitions when used with a gql tagged template literal. This is fixed by concatenating the document definitions—filtering for duplicates.

Without this, the following would fail:

import catFaceFragment from './catFaceFragment.gql';

const query = gql`
  query Kitten($name: String) {
    kitten(name: $name) {
      ... CatFace
    }
  }
  ${catFaceFragment}
`
#import "./whiskers.gql"

fragment CatFace on Cat {
  ... Whiskers
}
fragment Whiskers on Cat {
  whiskers {
    quantity
    length
    areWiderThanBody
  }  
}
Unknown fragment "Whiskers".

@apollo-cla
Copy link

@evanrs: Thank you for submitting a pull request! Before we can merge it, you'll need to sign the Meteor Contributor Agreement here: https://contribute.meteor.com/

@jnwng
Copy link
Contributor

jnwng commented Jun 21, 2017

@evanrs thank you!

however, i dont fully understand what's breaking here, is this the case where you're mixing usage of interpolation and the #import syntax?

if so, do you mind adding tests here to make sure this works as expected?

@jnwng
Copy link
Contributor

jnwng commented Jun 21, 2017

also, processFragments already deals with fragment de-duplication. can you see if we can re-use that or combine the logic so we dont have multiple ways of de-duplicating fragments?

@evanrs
Copy link
Author

evanrs commented Jun 21, 2017

Will do, thanks for the quick response.

@evanrs
Copy link
Author

evanrs commented Jun 21, 2017

@jnwng I've added a test that reproduces the problem.

I'm having trouble working this change into the current flow. processFragments relies on the fragmentDefinition having loc.source, but this is absent on definitions accumulated from the interpolated documents.

My attempt quickly cascaded to changes in both parseDocument and processFragments. Can you lend some direction on what you think is best here?

@AmauryLiet
Copy link

@evanrs I think merging your work would be highly valuable! Are you still working with graphql-tag on your project(s)?

@evanrs
Copy link
Author

evanrs commented Apr 21, 2018

Thanks, @AmauryLiet, I am.

Are you experiencing this issue?

@AmauryLiet
Copy link

@evanrs
Yes we were doing a refacto by switching from using gql`` tags in .js files to .graphql files and that made some requests fail ;)

Do you think this PR is mergeable or is it too old now?

@jnwng
Copy link
Contributor

jnwng commented Apr 23, 2018 via email

@mkochendorfer
Copy link

This looks like a great feature improvement. Anything I can do to help move this PR along?

@Vincz
Copy link

Vincz commented Jul 31, 2018

Hey guys! Any update on this? Can we help one way or another ? @evanrs @jnwng

@evanrs
Copy link
Author

evanrs commented Aug 1, 2018

@Vincz thanks for reviving this issue 🙂— the test is still not passing on master.

Given the interest in this @jnwng could you provide direction? Or, help us wrangle a member from @apollographql to take the lead on this?

@AmauryLiet, I think it is too old to accept this solution — other than the test case, which remains valid. If you, @Vincz, or @mkochendorfer want an edifying experience into GraphQL's ASTs I can promise creating a new PR on this issue will provide! 😛

@j0nd0n7
Copy link

j0nd0n7 commented Nov 13, 2018

I came to this issue from #159 because importing a fragment from a fragment doesn't work anymore.
Are these PR solve the issue? if so, could you move forward the PR?

@mzalewski
Copy link

Simple solution that seems to have worked for me:
https://github.com/mzalewski/graphql-tag/tree/patch-1

Any reason to avoid using the graphql language printer to do this?

@mzalewski
Copy link

Converted my code into a pull request (#227) - it's probably slightly inefficient since it's parsing, printing then parsing again - but it works for now and should fix the issue until someone figures out a better solution

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants